Conversation
WalkthroughAdds calendar-view functionality and refactors booking calendar internals. Header now accepts an optional Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx,js,jsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-08-29T22:57:31.407ZApplied to files:
🧬 Code graph analysis (1)packages/platform/atoms/event-types/wrappers/EventTypePlatformWrapper.tsx (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
Show resolved
Hide resolved
| useEffect(() => { | ||
| const searchParams = props.routingFormSearchParams | ||
| ? new URLSearchParams(props.routingFormSearchParams) | ||
| : new URLSearchParams(window.location.search); | ||
|
|
||
| const routedTeamMemberIds = getRoutedTeamMemberIdsFromSearchParams(searchParams); | ||
| const skipContactOwner = searchParams.get("cal.skipContactOwner") === "true"; | ||
|
|
||
| const _cacheParam = searchParams?.get("cal.cache"); | ||
| const _shouldServeCache = _cacheParam ? _cacheParam === "true" : undefined; | ||
| const isBookingDryRun = | ||
| searchParams?.get("cal.isBookingDryRun")?.toLowerCase() === "true" || |
There was a problem hiding this comment.
if we can in a followup pr let's make a custom hook for the search params and share the hook between components
There was a problem hiding this comment.
I think this is duplicated from booker platform wrapper
| searchParams?.get("cal.sandbox")?.toLowerCase() === "true"; | ||
| setRoutingParams({ | ||
| ...(skipContactOwner ? { skipContactOwner } : {}), | ||
| ...(routedTeamMemberIds ? { routedTeamMemberIds } : {}), | ||
| ...(_shouldServeCache ? { _shouldServeCache } : {}), | ||
| ...(isBookingDryRun ? { isBookingDryRun } : {}), | ||
| }); | ||
| }, [props.routingFormSearchParams]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
526-537: Localize or avoid hardcoded "Function not implemented." strings.If these surface, they should be behind t(); otherwise replace with no‑ops to avoid throwing in production.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/platform/atoms/booker/BookerPlatformWrapper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/platform/atoms/booker/BookerPlatformWrapper.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/platform/atoms/booker/BookerPlatformWrapper.tsx
🔇 Additional comments (1)
packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
598-603: Allow null/undefined and trim inputs in formatUsernameUse:
export function formatUsername(username: string | string[] | null | undefined): string { if (!username) return ""; if (typeof username === "string") return username.trim(); return username.filter(Boolean).map((u) => u.trim()).join("+"); }Before merging, confirm there are no other definitions/imports/usages of formatUsername by running locally:
rg -n --hidden -S 'export\s+(const|function)\s+formatUsername' --glob '!node_modules/' || true
rg -n --hidden -S 'import.*formatUsername' --glob '!node_modules/' || true
rg -n --hidden -S '\bformatUsername\b' --glob '!node_modules/**' || true
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable react-hooks/exhaustive-deps */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Remove file‑wide exhaustive‑deps disable; scope it to specific hooks.
A blanket disable can hide stale-closure bugs across this hook-heavy file. Prefer fixing deps or narrowly disabling per‑hook (you already do this in a few places).
Apply:
-/* eslint-disable react-hooks/exhaustive-deps */Run to audit where we still rely on blanket vs scoped disables:
🏁 Script executed:
#!/bin/bash
# Hooks and nearby eslint disables for this file
rg -n -C1 -e 'useEffect\(|useMemo\(|useCallback\(' packages/platform/atoms/booker/BookerPlatformWrapper.tsx
rg -n 'eslint-disable-next-line react-hooks/exhaustive-deps' packages/platform/atoms/booker/BookerPlatformWrapper.tsxLength of output: 2023
Remove file-wide eslint-disable react-hooks/exhaustive-deps. Scoped disables already cover the five specific hooks at lines 153, 158, 231, 468, and 490—drop the blanket disable at the top.
-/* eslint-disable react-hooks/exhaustive-deps */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* eslint-disable react-hooks/exhaustive-deps */ |
🤖 Prompt for AI Agents
In packages/platform/atoms/booker/BookerPlatformWrapper.tsx around line 1,
remove the file-wide comment "/* eslint-disable react-hooks/exhaustive-deps */"
and leave the existing scoped eslint-disable comments at lines 153, 158, 231,
468, and 490 intact; after removing the blanket disable, run ESLint (or your IDE
linter) to confirm there are no new react-hooks/exhaustive-deps warnings and
address any remaining specific hook warnings by scoping disables only to the
individual lines where necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx (5)
43-43: Consider using more precise type forviewprop.The default value
"MONTH_VIEW"suggests this should be typed askeyof typeof BookerLayoutsrather than allowing any string.Consider adding proper typing in the
typesfile for this prop to ensure type safety:- view = "MONTH_VIEW", + view = BookerLayouts.MONTH_VIEW,
107-108: Memoization has inconsistent dependencies.The
usernamedependency is included but the logic only uses the string length check which shouldn't change if username remains the same formatted value.- const isDynamic = useMemo(() => { - return getUsernameList(username ?? "").length > 1; - }, [username]); + const isDynamic = getUsernameList(username ?? "").length > 1;If memoization is truly needed for performance, consider memoizing the
getUsernameListcall itself.
180-189: Complex enabled condition could be simplified.The enabled condition combines multiple checks that could be extracted for readability and testability.
+ const isDataReady = props.isTeamEvent ? !isTeamPending : !isPending; + const hasRequiredIds = Boolean(teamId || username); + const hasSchedulingContext = Boolean(month) && Boolean(timezone) && Boolean(event?.data?.id); + enabled: - Boolean(teamId || username) && - Boolean(month) && - Boolean(timezone) && - (props.isTeamEvent ? !isTeamPending : !isPending) && - Boolean(event?.data?.id), + hasRequiredIds && hasSchedulingContext && isDataReady,
199-201: Magic numbers should be configurable or documented.The component uses hard-coded values (extraDays=7, nextSlots=6) without explanation or configuration options.
Consider making these configurable or at least documenting why these specific values were chosen:
+// Calendar view shows a week (7 days) at a time +const CALENDAR_VIEW_DAYS = 7; +// Navigation moves by 6 slots in column view +const COLUMN_VIEW_NAVIGATION_SLOTS = 6; <Header isCalendarView={true} isMyLink={true} eventSlug={eventSlug} enabledLayouts={bookerLayout.bookerLayouts.enabledLayouts} - extraDays={7} + extraDays={CALENDAR_VIEW_DAYS} isMobile={false} - nextSlots={6} + nextSlots={COLUMN_VIEW_NAVIGATION_SLOTS} />
195-196: Hard-coded calendar props OK for this wrapper; consider exposing them if reusedHeader uses isMyLink (controls the troubleshooter button) and isCalendarView (drives calendar-specific behavior like week-start / "today" logic). CalendarViewPlatformWrapper sets isCalendarView={true} and isMyLink={true} in packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx:195–196 — correct for the platform-only calendar. Expose these as props only if you expect this wrapper to be reused in other contexts (embed/mobile).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
🧬 Code graph analysis (1)
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx (10)
packages/platform/atoms/booker/types.ts (2)
BookerPlatformWrapperAtomPropsForIndividual(91-95)BookerPlatformWrapperAtomPropsForTeam(97-102)packages/platform/atoms/booker/BookerPlatformWrapper.tsx (1)
formatUsername(598-603)packages/features/bookings/Booker/BookerStoreProvider.tsx (3)
useBookerStoreContext(24-35)useInitializeBookerStoreContext(37-113)BookerStoreProvider(15-22)packages/platform/atoms/hooks/event-types/public/useAtomGetPublicEvent.tsx (1)
useAtomGetPublicEvent(23-65)packages/features/bookings/Booker/components/hooks/useBookerLayout.ts (1)
useBookerLayout(18-94)packages/platform/atoms/hooks/bookings/useGetBookingForReschedule.ts (1)
useGetBookingForReschedule(17-54)packages/platform/atoms/hooks/useAvailableSlots.ts (1)
useAvailableSlots(15-49)packages/features/bookings/Booker/components/Section.tsx (1)
BookerSection(43-63)packages/features/bookings/Booker/components/Header.tsx (1)
Header(21-175)packages/features/calendar-view/LargeCalendar.tsx (1)
LargeCalendar(15-79)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build API v2
- GitHub Check: Type check / check-types
- GitHub Check: Production builds / Build API v1
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
🔇 Additional comments (5)
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx (5)
32-34: Confirm the consolidated type approach is correct.The component accepts either individual or team props via union type, which looks good. The previous issue with
isTeamEvent?: falsepreventing passingtrueappears to have been addressed based on the commit message.
193-211: Good component structure with proper store context usage.The component properly uses the AtomsWrapper, BookerSection components with appropriate areas, and passes the required data to child components. The sticky positioning and z-index management look correct.
215-223: Clean wrapper implementation with proper store provider.The wrapper component correctly provides the BookerStore context that's required by the child component. This separation of concerns is well done.
117-136: Extract URL search params logic to custom hook.This duplicates logic from BookerPlatformWrapper. Consider creating a shared hook as suggested in the past review comment.
Create a custom hook
useBookingRoutingParamsto share this logic between CalendarViewPlatformWrapper and BookerPlatformWrapper:export function useBookingRoutingParams(routingFormSearchParams?: URLSearchParams | string) { return useMemo(() => { const searchParams = routingFormSearchParams ? new URLSearchParams(routingFormSearchParams) : new URLSearchParams(window.location.search); const routedTeamMemberIds = getRoutedTeamMemberIdsFromSearchParams(searchParams); const skipContactOwner = searchParams.get("cal.skipContactOwner") === "true"; const _cacheParam = searchParams?.get("cal.cache"); const _shouldServeCache = _cacheParam ? _cacheParam === "true" : undefined; const isBookingDryRun = searchParams?.get("cal.isBookingDryRun")?.toLowerCase() === "true" || searchParams?.get("cal.sandbox")?.toLowerCase() === "true"; return { ...(skipContactOwner ? { skipContactOwner } : {}), ...(routedTeamMemberIds ? { routedTeamMemberIds } : {}), ...(_shouldServeCache ? { _shouldServeCache } : {}), ...(isBookingDryRun ? { isBookingDryRun } : {}), }; }, [routingFormSearchParams]); }
155-156: Clarify semantics and precedence ofrescheduleUidvsbookingUid.Search returned no repo matches here — confirm the intended rule when both props are supplied. Either document that
rescheduleUidoverridesbookingUid(add an inline comment and tests), or make the props mutually exclusive / consolidate/rename them to remove ambiguity.
Location: packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx (props mapping).
| const monthCount = | ||
| ((bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time") || | ||
| bookerLayout.layout === BookerLayouts.COLUMN_VIEW) && | ||
| dayjs(date).add(1, "month").month() !== | ||
| dayjs(date).add(bookerLayout.columnViewExtraDays.current, "day").month() | ||
| ? 2 | ||
| : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex month count calculation needs documentation.
This complex conditional logic for determining monthCount is difficult to understand without context.
Add explanatory comment and consider extracting to a helper function:
+ // Determine if we need to prefetch two months of data when the view spans across month boundaries
+ const shouldPrefetchTwoMonths = () => {
+ const isTimeSelectionInNonWeekView =
+ bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time";
+ const isColumnView = bookerLayout.layout === BookerLayouts.COLUMN_VIEW;
+
+ if (!isTimeSelectionInNonWeekView && !isColumnView) return false;
+
+ const viewEndDate = dayjs(date).add(bookerLayout.columnViewExtraDays.current, "day");
+ return dayjs(date).add(1, "month").month() !== viewEndDate.month();
+ };
+
- const monthCount =
- ((bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time") ||
- bookerLayout.layout === BookerLayouts.COLUMN_VIEW) &&
- dayjs(date).add(1, "month").month() !==
- dayjs(date).add(bookerLayout.columnViewExtraDays.current, "day").month()
- ? 2
- : undefined;
+ const monthCount = shouldPrefetchTwoMonths() ? 2 : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const monthCount = | |
| ((bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time") || | |
| bookerLayout.layout === BookerLayouts.COLUMN_VIEW) && | |
| dayjs(date).add(1, "month").month() !== | |
| dayjs(date).add(bookerLayout.columnViewExtraDays.current, "day").month() | |
| ? 2 | |
| : undefined; | |
| // Determine if we need to prefetch two months of data when the view spans across month boundaries | |
| const shouldPrefetchTwoMonths = () => { | |
| const isTimeSelectionInNonWeekView = | |
| bookerLayout.layout !== BookerLayouts.WEEK_VIEW && bookerState === "selecting_time"; | |
| const isColumnView = bookerLayout.layout === BookerLayouts.COLUMN_VIEW; | |
| if (!isTimeSelectionInNonWeekView && !isColumnView) return false; | |
| const viewEndDate = dayjs(date).add(bookerLayout.columnViewExtraDays.current, "day"); | |
| return dayjs(date).add(1, "month").month() !== viewEndDate.month(); | |
| }; | |
| const monthCount = shouldPrefetchTwoMonths() ? 2 : undefined; |
🤖 Prompt for AI Agents
In
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
around lines 76 to 82, the conditional computing monthCount is hard to read and
lacks context; extract this logic into a well-named helper (e.g.,
computeMonthCountForLayout) and add an explanatory comment above the helper
explaining the business intent (when to show 2 months: selecting_time in
non-week layouts or column view and the start and end dates fall in different
months). Move the dayjs arithmetic and boolean checks into the helper, return 2
or undefined, and replace the inline expression with a clear call to the helper
so the intent is documented and the component code is easier to read and test.
| }, [props.routingFormSearchParams]); | ||
| const bookingData = useBookerStoreContext((state) => state.bookingData); | ||
| const setBookingData = useBookerStoreContext((state) => state.setBookingData); | ||
| const layout = BookerLayouts[view]; |
There was a problem hiding this comment.
Validate view prop value.
The view prop is used to index into BookerLayouts without validation, which could cause runtime errors if an invalid value is passed.
Add validation to ensure the view is valid:
- const layout = BookerLayouts[view];
+ const layout = BookerLayouts[view] || BookerLayouts.MONTH_VIEW;Or better yet, validate earlier and use proper typing as suggested in line 43.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const layout = BookerLayouts[view]; | |
| const layout = BookerLayouts[view] || BookerLayouts.MONTH_VIEW; |
🤖 Prompt for AI Agents
In
packages/platform/atoms/calendar-view/wrappers/CalendarViewPlatformWrapper.tsx
around line 139, the code indexs BookerLayouts with view without validation;
update the component to ensure view is a valid key of BookerLayouts (preferably
by changing the prop/type at line 43 to view: keyof typeof BookerLayouts), and
add a runtime guard before using it: check if view exists in BookerLayouts and
either select a safe default layout or throw/log a clear error if invalid; this
ensures no runtime undefined lookup and aligns runtime checks with the stronger
TypeScript typing.
E2E results are ready! |
* fix: refactor * add `isMonthViewProp` to header * feat: init v1 for calendar view atom * test breaking toggle buttons * fix: make sure week start is always sunday for calendar view atom * fixup * fix: remove extra comments * fix: add calendar view page in examples app * chore: add changesets * fix: coderabbit feedback * fixup
What does this PR do?
Image Demo:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This can be tested in the web app, i've created a new page for this called
/calendar-view